fix: split CORS policy across public, credentialed, and admin routes - #316

@bamiebot-maker Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

One line from green. The drift guard is the only thing failing:

Found 1 environment variable(s) read in source but missing from .env.example and not in allow-list:
  - CORS_PUBLIC_BLOCKED_ORIGINS (read at src/cors.ts:120)

Add it to .env.example with a comment saying what it does and what the format is (comma-separated origins, presumably, and what an empty value means). The guard exists because a variable that only lives in someone's shell is a variable the next operator does not know to set.

Splitting CORS across public, credentialed and admin surfaces is the right shape — a single permissive policy in front of /admin/* is a real problem — so I want this. I will review it properly on the next green run.

Hi @Miracle656, thanks for the review! I have added \CORS_PUBLIC_BLOCKED_ORIGINS\ to .env.example\ with documentation describing its format (comma-separated origins) and behavior (empty/unset permits all public cross-origin requests). \envDrift.test.ts\ and all 20 CORS tests are now green.

Merged as 9374b11. The structure is right and both restricted tiers default to origin: false, so an unconfigured deployment fails closed rather than open — which is the single most important decision in a change like this and the one most often got backwards.

The best idea in the PR is classifying on the request, not only on the path:

const authHeader = headers['authorization']
const cookieHeader = headers['cookie']
if (authHeader || cookieHeader) return 'credentialed'

const requestHeaders = headers['access-control-request-headers']
if (typeof requestHeaders === 'string') {
  const lower = requestHeaders.toLowerCase()
  if (lower.includes('authorization') || lower.includes('cookie')) return 'credentialed'
}

Checking the preflight's Access-Control-Request-Headers as well as the real request is the detail that makes it work — a preflight carries no Authorization, so a path-only classifier hands the browser an open policy for a call that will arrive authenticated. Good catch.

Merged locally; one conflict in src/app.ts against the fiat routes, global rate limit and ETag registration that landed since, resolved as a union.

Four changes on top.

1. classifyRoute tested the raw path, so an admin route could be spelled into the public tier

const normalizedPath = (path || '/').split('?')[0].replace(/\/+$/, '') || '/'
if (normalizedPath === '/admin' || normalizedPath.startsWith('/admin/')) return 'admin'

req.url is the client's spelling. find-my-way percent-decodes before matching, so:

'/%61dmin/keys'.startsWith('/admin/')                  →  false
decodeURIComponent('/%61dmin/keys').startsWith('/admin/') →  true

The request reaches the admin handler and gets the public policy: origin: true, open to any site. This is the same bypass #574 fixed in the admin auth hook earlier today — worth knowing it generalises to anything that pattern-matches req.url.

Fixed by decoding until stable, so a double encoding cannot survive one pass, with a malformed escape left alone rather than throwing:

let decoded = (path || '/').split('?')[0]
for (let i = 0; i < 3; i++) {
  let next: string
  try { next = decodeURIComponent(decoded) } catch { break }
  if (next === decoded) break
  decoded = next
}

2. CORS_ALLOWED_ORIGINS=* echoed any origin back alongside credentials: true

if (norm === '*' || norm === normalizedOrigin) return true

In the credentialed tier that produces Access-Control-Allow-Origin: <whatever the caller sent> with credentials: true, which is the configuration you cannot write by accident in a static config and should not be able to write here either: any site can drive an authenticated cross-origin call with the browser's credentials attached. Browsers reject a literal * with credentials, which is exactly why echoing the origin is the dangerous form — it looks compliant and is not.

isOriginAllowed now takes allowWildcard, false for credentialed and admin, true for public reads (open to everything anyway and sending no credentials).

3. A wildcard entry ignored its own scheme

const domainPart = norm.replace(/^https?:\/\/\*\./, '').replace(/^\*\./, '')

https://*.example.com had the scheme stripped before comparison, so it matched http://evil.example.com too — an https-only allow-list that silently permits plaintext. A schemed entry now has to match the scheme; a scheme-less *.example.com still matches either, which is what writing no scheme asks for. And an unparseable Origin is refused rather than string-compared.

4. ADMIN_ROUTES, CREDENTIALED_ROUTES, PUBLIC_ROUTES were dead

Nothing in production reads them — classifyRoute works from prefixes and headers — and the only references anywhere were:

expect(ADMIN_ROUTES).toContain('/admin/keys')
expect(CREDENTIALED_ROUTES).toContain('/usage/me')
expect(PUBLIC_ROUTES).toContain('/status')

which asserts that arrays contain what they were written to contain. A hard-coded route inventory that nothing enforces is precisely the failure mode #229 just finished removing from the README: it will be wrong within a month and will read as authoritative while it is. Removed, with a note in their place saying why there is deliberately no list.

I replaced that test with four that pin the fixes above: the percent-encoded admin path (single and double encoded), the wildcard refusal per tier, scheme matching in both directions, and an unparseable origin. 23 cases in cors.test.ts now, all passing.

One thing worth a follow-up

classifyRoute derives the tier from path prefixes, but since #574 the route itself declares its tier:

{ config: { admin: true, auth: 'admin' } }   // admin routes
{ config: { public: true, auth: 'none' } }   // genuinely public

That is a better source of truth than a prefix test, for the same reason the route inventories had to go: there is no second thing to keep in sync, and a route that moves out from under /admin keeps its classification. The catch is ordering — the CORS delegator runs before req.routeOptions is reliably populated for a preflight, so it cannot simply read the config today. Worth an issue: either consult routeOptions.config when it is available and fall back to the prefix test, or register the delegator so it runs after routing. Happy to assign it to you if you want it.

129 test files / 1,553 tests pass. tsc --noEmit and npm run build clean.